Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: improved permission handling for MapFormField #1228

Merged
merged 4 commits into from
Nov 11, 2024

Conversation

PeterBaker0
Copy link
Contributor

@PeterBaker0 PeterBaker0 commented Nov 11, 2024

feat: improved permission handling for MapFormField

This PR enables a center to be defined while still requesting and gracefully handling a current location call (which prompts permission check). This is to allow user testing with the following workflow.

This might be temporary since in reality we probably don't need to ask for the user's location if we don't intend to actually use it.

Technical changes include:

  • useEffect with useRef to only run once
  • use const variables instead of modifying readonly props
  • only store get location result if center is not defined in props
  • add alerts for warning conditions such as not being able to access location
  • add alerts for using default map center location (no current location available AND no configured center location) - this defaults to Sydney

How to Test

Try different permutations of permissions yes, no, ask, as well as providing and not providing a center in the notebook definition. Check it always respects the center provided, shows errors where appropriate, and notifications work.

Checklist

  • I have confirmed all commits have been signed.
  • I have added JSDoc style comments to any new functions or classes.
  • Relevant documentation such as READMEs, guides, and class comments are updated.

…f modifying readonly props, only store get location result if center is not defined in props

Signed-off-by: Peter Baker <[email protected]>
Copy link
Contributor

@stevecassidy stevecassidy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A reasonable solution but I think you've lost the supplied label.

app/src/gui/fields/maps/MapFormField.tsx Show resolved Hide resolved
app/src/gui/fields/maps/MapFormField.tsx Show resolved Hide resolved
@PeterBaker0 PeterBaker0 merged commit 79d1b43 into main Nov 11, 2024
2 checks passed
@PeterBaker0 PeterBaker0 deleted the improved-permissions-testing-workflow branch November 11, 2024 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants